TNO-1136 Set tone by user #2251

areyeslo · 2024-09-04T04:02:35Z

Feature Update: User-Defined Tone Assessment for Articles

This update introduces the ability for subscribers to assess and set a personal tone for an article, independent of the original tone. The custom tone selected by the user will be saved and associated with their account, ensuring that their personal assessment is retained without altering the article's original tone data.

User Tone:

Editor Tone:

In DB, a record is created on tone_pool table which is used to tone any content. When the user set a tone for a content, it is saved in content_tone table.

Story: https://citz-gdx.atlassian.net/browse/TNO-1136

Fosol · 2024-09-04T13:00:32Z

api/net/Areas/Subscriber/Controllers/TonePoolController.cs

+    [ProducesResponseType(typeof(ErrorResponseModel), (int)HttpStatusCode.BadRequest)]
+    [SwaggerOperation(Tags = new[] { "TonePool" })]
+    [ETagCacheTableFilter("tone_pools")]
+    [ResponseCache(Duration = 5 * 60)]


Doesn't make sense on a POST endpoint

Fosol · 2024-09-04T13:00:35Z

api/net/Areas/Subscriber/Controllers/TonePoolController.cs

+    [ProducesResponseType(typeof(TonePoolModel), (int)HttpStatusCode.Created)]
+    [ProducesResponseType(typeof(ErrorResponseModel), (int)HttpStatusCode.BadRequest)]
+    [SwaggerOperation(Tags = new[] { "TonePool" })]
+    [ETagCacheTableFilter("tone_pools")]


Doesn't make sense on a POST endpoint

Fosol · 2024-09-04T13:01:19Z

api/net/Areas/Subscriber/Controllers/TonePoolController.cs

+    [ProducesResponseType(typeof(IEnumerable<TonePoolModel>), (int)HttpStatusCode.OK)]
+    [ProducesResponseType((int)HttpStatusCode.NotModified)]
+    [SwaggerOperation(Tags = new[] { "TonePool" })]
+    [ETagCacheTableFilter("tone_pools")]


Doesn't make sense on an endpoint that returns one item.

Fosol · 2024-09-04T13:01:31Z

api/net/Areas/Subscriber/Controllers/TonePoolController.cs

+    [ProducesResponseType((int)HttpStatusCode.NotModified)]
+    [SwaggerOperation(Tags = new[] { "TonePool" })]
+    [ETagCacheTableFilter("tone_pools")]
+    [ResponseCache(Duration = 5 * 60)]


Have you tested this?

I have not tested findbyId, just findByUserId. Going to test it.

Fosol · 2024-09-04T13:04:58Z

app/subscriber/src/features/my-reports/edit/content/ContentEditForm.tsx

+            name: `${userId}`,
+            ownerId: userId,
+          });
+          const newTonePool = await getMyTonePool(userId);


Extra request isn't needed, just use the returned tonepool from the addMyTonePool.

Fosol · 2024-09-04T13:32:54Z

libs/net/entities/Models/ContentVersion.cs

+    /// <summary>
+    /// get/set - Tone for the content
+    /// </summary>
+    public int? Tone {get;set;}


Are you even using this property?

No, this is from the previous implementation. Thank you!

Fosol · 2024-09-04T13:36:40Z

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx

+          {() => (
+            <Form>
+              <FormikSentiment
+                name="tonePools"


If you correctly named this component then the Formik setFieldValue would work correctly. and you wouldn't need also include the onSentimentChange logic.

I'd need to check this by chat. I'd love to implement this way :)

Not using FormikSentiment component.

Fosol · 2024-09-04T13:38:09Z

libs/npm/core/src/components/formik/sentiment/FormikSentiment.tsx

+      updatedTonePools = [...(tonePools || []), { ...defaultTonePool, value: option }];
+    }
+
+    setFieldValue(name.toString(), updatedTonePools, true);


Having both of these can result in odd state. Formik will update the form context and then another separate set of state is being updated next. It is easier to maintain if you make the setFieldValue work correctly.

Not using FormikSentiment.

Fosol · 2024-09-04T13:41:24Z

libs/npm/core/src/hooks/api/subscriber/useApiSubscriberTonePools.ts

+  const api = useApi(options);
+
+  return React.useRef({
+    getTonePools: (etag: string | undefined = undefined) => {


This endpoint doesn't appear to exist.

Fosol · 2024-09-04T13:43:15Z

libs/npm/core/tsconfig.json

-    ],
+    "lib": ["dom", "dom.iterable", "esnext"],
+    "paths": {
+      "@assets/*": ["../../../../app/subscriber/public/assets/*"]


I don't understand the need for this? Shouldn't we keep tno-core assets locally? If it's the shared library then the apps that use the shared library should be getting their components/assets from tno-core. It is also easier to maintain.

Thank you for bringing this, I wanted to use the proper icons instead of applying style:
<FaSmile className="tone-icon" color="#59E9BE" />
The icons are under app/subscriber/public/assets/reports/, so I created an alias to be used in the import. Then I could use import @reports/[email protected]

areyeslo force-pushed the TNO-1136-SetToneByUser branch 2 times, most recently from 2967acf to bc45f09 Compare September 4, 2024 04:15

Fosol assigned areyeslo Sep 4, 2024

Fosol added enhancement New feature or request subscriber PR contains changes towards the subscriber application, labels Sep 4, 2024

Fosol requested changes Sep 4, 2024

View reviewed changes

areyeslo force-pushed the TNO-1136-SetToneByUser branch 16 times, most recently from a42d911 to 95d7d11 Compare September 5, 2024 19:43

areyeslo requested a review from Fosol September 5, 2024 19:54

areyeslo force-pushed the TNO-1136-SetToneByUser branch from 95d7d11 to 13be7df Compare September 5, 2024 20:59

areyeslo added the tno-core update Indicates that there have been changes to our tno-core package, which can pose concurrency issues. label Sep 5, 2024

areyeslo force-pushed the TNO-1136-SetToneByUser branch from 13be7df to c7b383f Compare September 5, 2024 21:18

areyeslo and others added 3 commits September 5, 2024 14:42

User can set toning.

347a22d

Code review changes.

7aaff82

Publish tno-core:0.1.146

448d600

Fosol force-pushed the TNO-1136-SetToneByUser branch from c7b383f to 448d600 Compare September 5, 2024 21:48

Fosol approved these changes Sep 5, 2024

View reviewed changes

areyeslo merged commit 3187930 into dev Sep 5, 2024
7 checks passed

areyeslo deleted the TNO-1136-SetToneByUser branch September 5, 2024 22:18

areyeslo mentioned this pull request Sep 6, 2024

Retrieve previous version of package.json and yarn.lock #2268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TNO-1136 Set tone by user #2251

TNO-1136 Set tone by user #2251

areyeslo commented Sep 4, 2024 •

edited

Loading

Fosol Sep 4, 2024

areyeslo Sep 4, 2024

Fosol Sep 4, 2024

Fosol Sep 4, 2024

Fosol Sep 4, 2024

areyeslo Sep 4, 2024

Fosol Sep 4, 2024

Fosol Sep 4, 2024

areyeslo Sep 4, 2024

Fosol Sep 4, 2024

areyeslo Sep 4, 2024

areyeslo Sep 5, 2024

Fosol Sep 4, 2024

areyeslo Sep 5, 2024

Fosol Sep 4, 2024

Fosol Sep 4, 2024

areyeslo Sep 4, 2024

TNO-1136 Set tone by user #2251

TNO-1136 Set tone by user #2251

Conversation

areyeslo commented Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areyeslo commented Sep 4, 2024 •

edited

Loading